Repository navigation
wallet: Close incomplete descriptor rescans - #113
BenWestgate wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7181c7b70e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d53a4a426
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1863df760
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d65adc189
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Codex), posted at the maintainer's request.
Not ACKing 175cf4c yet: the GUI budget is documented consistently at 2,050 now, but its required explicit maintainer authorization is still missing.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 175cf4c: one existing P2 remains. The GUI budget is now documented as 2,050 lines, but the repository contract requires explicit maintainer authorization for a budget increase and none is recorded.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review of 175cf4c: one blocking documentation/security-contract issue. The new Before you start text says the separate wallet record “proves later that cards you restore are this wallet.” That overstates the settled threat model: the 32-bit master fingerprint is a human-scale accident-safety check for wrong/mixed cards and transcription/correction mistakes; it is not malicious-tampering authentication. Please change this to wording such as “helps confirm later that the restored cards match your recorded wallet before import,” while keeping the stronger authenticated descriptor-backup work separate. I found no other blocker in the bundled-form launcher, package-data boundary, handwriting key, or pre-seed checklist flow.
207c0f4 to
e43102f
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI current-head review: ACK bd49e52 for this branch's scope. The checklist occurs before secret generation, only bundled blank forms are handed to the desktop, GTK <4.10 takes the explicit path fallback, package data includes both forms, and CODEX32_FORMS_DIR cleanly supplies a confinement-readable copy. The remaining Tails integration dependency is already explicit: Bails must copy the forms into a Tor-Browser-readable directory and set that variable when it repins. No additional code finding here.
|
cNACK put a dot in each 6, there's no evidence this is a standard way to disambiguate these letters, we can simply say to make G's horizontal bar obvious and open, while to close the loops of 6 and 9 |
|
Done in d08bbbe. The dot is gone. The window, the printable recovery card and Generated by Claude Code |
|
please read CONTRIBUTING.md and squash this into atomic easy to review commits before human review. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: Concept ACK d08bbbe. The form preflight precedes seed generation; the corrected handwriting key is consistent. The #139 changes gate dated restore on Core's prune/AssumeUTXO state before import and keep new wallets loaded on startup, with focused tests. Not ready for human review until the requested atomic commit cleanup; also test the complete restore and form-opening flow on supported Tails.
d08bbbe to
aea9da6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aea9da63e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if core._target(name) is None or any(type(w) is not str or "could not be updated" in w for w in notes): | ||
| raise BitcoinCoreError("Bitcoin Core did not create an empty wallet that it loads at every start.") |
There was a problem hiding this comment.
When createwallet succeeds but reports that load_on_startup could not be saved, this raises only after Core has already created and loaded the blank wallet. Core's createwallet contract returns the created wallet name together with any warnings, so the wallet remains eligible to _wallets; after choosing Start again or Restore my wallet, the operator can select and fill it even though it will not load after a restart. On a pruned node that wallet can then fall behind the retained blocks, defeating the protection this option was added to provide. Keep this wallet out of subsequent selection, or otherwise ensure it cannot be filled until startup persistence is confirmed.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
report Core's warning is sufficient.
| if mismatch: | ||
| return _fingerprint_page(view, core, secret, timestamp, restoring=restoring, problem=mismatch) | ||
| _wallets(view, core, secret, timestamp, expected, restoring=restoring) | ||
| _wallets(view, core, secret, start, expected, restoring=restoring) |
There was a problem hiding this comment.
When restoration uses a nonblank creation date, this line passes start—one day before that date—to importdescriptors; Core documents that the rescan is based on the earliest supplied timestamp. The resulting _RESTORED screen nevertheless tells the operator that Core is scanning “from the beginning.” A late or mistyped approximate date can therefore omit older transactions while the completion screen falsely implies that all chain history was checked; describe the selected dated scan instead, or retain timestamp zero if promising a genesis scan.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
describe the selected dated scan instead
| kept = t if isinstance(b, dict) and type(t := b.get("time")) is int and t >= genesis else 0 | ||
| if start < kept + 86400 or not kept: | ||
| since = f" from before {time.strftime('%Y-%m-%d', time.gmtime(kept))}" if kept else "" |
There was a problem hiding this comment.
If the untrusted getblockstats response contains a very large integer such as 10**20 for time, it passes the current lower-bound check and reaches time.gmtime(kept), which raises OverflowError on supported platforms instead of the promised BitcoinCoreError. The CLI does not catch that exception and can terminate with a traceback during wallet initialization, while the GUI reports only its generic unexpected-failure message. Apply a sane upper bound to the retained-block timestamp, or translate failures from the date conversion into BitcoinCoreError.
AGENTS.md reference: AGENTS.md:L78-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Apply a sane upper bound to the retained-block timestamp
aea9da6 to
042fcf1
Compare
BenWestgate
changed the title
042fcf1 to
b045908
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b045908d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| kept = b.get("time") if isinstance(b, dict) else None | ||
| if type(kept) is not int or not genesis <= kept < time.time() + 7200: | ||
| raise BitcoinCoreError("Bitcoin Core did not say which blocks it still keeps.") | ||
| if start < kept + 86400: |
There was a problem hiding this comment.
When the first retained block's raw timestamp lags a pruned predecessor's timestamp by more than roughly one day—a reproducible regtest case after a mock-time gap and valid under consensus rules—this comparison can approve a date even though Core will start the rescan in pruned history. RescanFromTime selects the first block using cumulative GetBlockTimeMax, not the prune-height block's raw time; importdescriptors can therefore fail after modifying the destination wallet, defeating this preflight. Base the gate on the same cumulative maximum or verify that Core's actual starting block is retained.
AGENTS.md reference: AGENTS.md:L78-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Base the gate on the same cumulative maximum
There was a problem hiding this comment.
Core doesn't expose the cumulative maximum over RPC. GetBlockTimeMax is only reachable through FindEarliestAtLeast, and the only RPC that calls that is pruneblockchain, which deletes blocks. getblockheader and getblockstats return only raw time and mediantime. Raw times aren't monotonic, so computing the maximum ourselves means reading every header below the prune height. That's one bitcoin-cli call per block, hundreds of thousands on a pruned mainnet node.
The exact signal we do get is Core's own per-descriptor "Rescan failed … pruning" error from importdescriptors. Today that surfaces as the generic "did not import every private descriptor". I'm proposing to keep the cheap preflight as a first filter, and to turn that error into a clear "this pruned node lacks blocks from your date" failure that unloads the wallet the restore just created. The alternatives are to keep the one-day margin as is, or to walk all the headers. I'm asking which you want in the project thread, so this reply doesn't stand in for that choice.
Generated by Claude Code
There was a problem hiding this comment.
Done in 59d7b3c, following the option you picked. The cheap preflight stays. When importdescriptors reports "Rescan failed", initialize now runs unloadwallet <name> false, so the wallet isn't loaded on startup, and raises a message saying the wallet is missing history and was closed. That replaces the generic import failure. A test covers it. The library is at 5,019 lines, inside the 5,025 budget. This is one commit on top, ready to squash into "wallet: Rescan from recorded creation date".
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59d7b3cb6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """Create one blank descriptor wallet and return Core's warnings for the operator.""" | ||
| # Let Core persist startup loading so the wallet follows new blocks. | ||
| arguments = [f"wallet_name={_wallet_name(name)}", "disable_private_keys=false", "blank=true"] | ||
| arguments.append("load_on_startup=true") |
There was a problem hiding this comment.
When restoring on a pruned node into an eligible wallet that was loaded transiently (for example, with loadwallet name false), this persistence flag never runs: _wallet_page sends existing choices directly to _import, so the dated restore can succeed but the wallet disappears after the next Core restart and can fall behind blocks that are subsequently pruned. Bitcoin Core defines false as removing the wallet from its persistent startup list in the loadwallet contract. Ensure selected existing destinations are persistent, or reject/warn for pruned restores, rather than protecting only wallets created here. This differs from the already-discussed createwallet-warning case because this path makes no createwallet call and produces no persistence warning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This isn't something this PR introduced: the existing-wallet path has never set load_on_startup. Core has no RPC that changes that flag on a wallet that's already loaded. The only way is unloadwallet followed by loadwallet name true, which would close a wallet the operator opened themselves. Bails also already reloads every wallet with load_on_startup on each Core start (Bails #315, load-wallets). I'm leaving this for Ben to decide rather than widening this PR.
Generated by Claude Code
59d7b3c to
b63b112
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b63b112ba9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b63b112 to
aaff90f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaff90fb33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Core starts a dated rescan at the first block whose highest time so far reaches the date. No RPC reports that cumulative time, so the pruning check reads the oldest kept block's own time, and a pruned block that ran more than a day ahead can still pass it. Core then imports the keys, reports "Rescan failed" for each descriptor, and leaves a wallet that is missing history. When that happens, ask Core to unload the wallet and drop it from startup, and say why instead of the generic import failure. The message tells the operator not to use the wallet and to close it whenever it is open, since the unload can fail or be undone at the next start. It advises a later date only when Core blames pruning; an aborted scan needs the same date again. The GUI's relock treats a wallet that a successful listing no longer shows as locked, so it does not hide that message, and still asks for a check if Core stops answering. Shorten the blank wallet prompts and the createwallet check to stay inside the library and GUI line budgets. Refs BenWestgate/Bails#314
Pressing Ctrl-C at the first-use date prompt of create --existing reached the generic create handler, which tells the operator to void every card from the incomplete creation. Nothing has been made at that point, and the only cards in play are the operator's existing, valid backup. Report a plain interruption there instead. Refs BenWestgate/Bails#314
aaff90f to
6c64411
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review.
Code ACK 6c64411 (static). The new rescan-failure path preserves the warning if unload fails and relocks the wallet; interrupting an existing-seed date prompt no longer voids valid cards. Targeted regression tests are present and CI passes. Before human review, update the PR body: it says two commits, but this head has four. Supported-Tails GUI/restore testing remains outstanding.
|
cACK commit 1,2, and 4 aNACK I'm not reviewing +856 lines CONTRIBUTING.md says:
Consider breaking these apart into smaller focused PRs, do less, shrink the diffs, and/or simplify your changes to make an easy review practical. |
BenWestgate
changed the title
|
AI-assisted response to review feedback: agreed. Split into #190 (forms), #191 (dated restore), and this PR (#113), now just 2 commits / +121 −30 lines; no force-push. Commit 3 is needed because non-monotonic block timestamps can defeat the pruning preflight. Core then reports Existing tests and CI cover the code; supported-Tails/pruned-Core end-to-end testing remains open. |
|
+121 is a lot of code for what seems like a simple bounded problem of how "off" the time reported by the RPC can be. Are you sure there is not a MUCH simpler solution like being conservative about which blocks we have so that this doesn't need so much edge case handling? |
|
AI-assisted response to the size concern: no fixed timestamp margin guarantees a safe rescan on pruned Core. Core's selection depends on the maximum preceding block time, which the preflight RPCs do not expose; failed rescans can also have other causes. A larger buffer could reduce failures but cannot replace handling |
Why
A pruning preflight is not conclusive: Bitcoin Core chooses the rescan start using the highest prior block timestamp, but the API exposes only individual block times. Non-monotonic timestamps can let the preflight pass even when the actual descriptor rescan later reports
Rescan failed. In that case Core has already imported keys into a wallet missing transaction history.Change
6afb94f: onRescan failed, tryunloadwallet <name> false(also remove from startup); retain the original diagnosis if unload fails or warns, and instruct the operator to close and replace the incomplete wallet. Preserve encrypted-wallet relock warnings.6c64411: Ctrl-C at thecreate --existingfirst-use date prompt must not instruct users to discard valid recovery cards.Review
This is now a two-commit PR stacked on #191 (which stacks on #190). The original four commits are unchanged; the large preparation and dated-rescan changes have dedicated PRs. The earlier merged-to-branch #139 review covers the date work. Relevant unit tests and exact-head CI previously passed, but supported-Tails/pruned-Core integration testing is still required. Prepared with AI assistance; final human review required.